Fix building auth metadata paths#779
Conversation
|
|
Thank you for the suggestion @Kludex. Refactored the |
src/mcp/server/auth/routes.py
Outdated
| issuer_url, lambda path: path.rstrip("/") + TOKEN_PATH.lstrip("/") | ||
| issuer_url, lambda path: append_path(path, AUTHORIZATION_PATH) | ||
| ) | ||
| token_url = modify_url_path(issuer_url, lambda path: append_path(path, TOKEN_PATH)) |
There was a problem hiding this comment.
I think we can delete this modify_url_path tho. str(issuer_url) should work.
There was a problem hiding this comment.
I was trying to modify as little code as possible, but you're right, this can be simplified by just casting the URL to a string and merging it with the endpoint (authorize, token, etc...)
I decided to convert the merged endpoint into a AnyHttpUrl pydantic class because the OAuthMetadata class is expecting AnyHttpUrl URLs
Let me know what you think! @Kludex
| authorization_url = modify_url_path( | ||
| issuer_url, lambda path: path.rstrip("/") + AUTHORIZATION_PATH.lstrip("/") | ||
| authorization_url = AnyHttpUrl( | ||
| str(issuer_url).rstrip("/") + AUTHORIZATION_PATH |
There was a problem hiding this comment.
There's no need to strip in any of those.
| str(issuer_url).rstrip("/") + AUTHORIZATION_PATH | |
| urljoin(str(issuer_url), AUTHORIZATION_PATH) |
There was a problem hiding this comment.
Thanks for the suggestion, @Kludex!
I initially thought the same — that urljoin(str(issuer_url), "/authorize") would be enough. But it actually breaks in some cases where the base_url includes a path.
For example, when the base_url is https://example.com/auth/oidc/op/Customer/, urljoin returns https://example.com/authorize, which drops the intended path entirely. That’s because urljoin treats the /authorize as an absolute path and replaces everything after the domain.
To illustrate this, I added a test suite comparing both approaches:
import unittest
from urllib.parse import urljoin
from pydantic import AnyHttpUrl
class TestModifyUrlPath(unittest.TestCase):
def test_append_authorize_to_urls_with_urljoin(self):
"""Test appending /authorize to various URL formats using urljoin"""
test_cases = [
("https://example.com", "https://example.com/authorize"),
("https://example.com/", "https://example.com/authorize"),
("https://example.com/auth/oidc/op/Customer", "https://example.com/auth/oidc/op/Customer/authorize"),
("https://example.com/auth/oidc/op/Customer/", "https://example.com/auth/oidc/op/Customer/authorize"),
("http://localhost:8000", "http://localhost:8000/authorize"),
("http://localhost:8000/", "http://localhost:8000/authorize"),
]
for base_url, expected in test_cases:
any_http_url = AnyHttpUrl(base_url)
with self.subTest(base_url=any_http_url):
result = AnyHttpUrl(urljoin(str(any_http_url), "/authorize"))
self.assertEqual(result, AnyHttpUrl(expected))
def test_append_authorize_to_urls_with_rstrip(self):
"""Test appending /authorize to various URL formats using rstrip"""
test_cases = [
("https://example.com", "https://example.com/authorize"),
("https://example.com/", "https://example.com/authorize"),
("https://example.com/auth/oidc/op/Customer", "https://example.com/auth/oidc/op/Customer/authorize"),
("https://example.com/auth/oidc/op/Customer/", "https://example.com/auth/oidc/op/Customer/authorize"),
("http://localhost:8000", "http://localhost:8000/authorize"),
("http://localhost:8000/", "http://localhost:8000/authorize"),
]
for base_url, expected in test_cases:
any_http_url = AnyHttpUrl(base_url)
with self.subTest(base_url=any_http_url):
result = AnyHttpUrl(str(any_http_url).rstrip("/") + "/authorize")
self.assertEqual(result, AnyHttpUrl(expected))
if __name__ == "__main__":
unittest.main()So for consistency across all cases, I believe we need to keep the rstrip("/") approach.
Let me know what you think!
There was a problem hiding this comment.
Unfortunate, but it makes sense.
|
I'll merge this once #806 lands. I need an approval from another maintainer there. |
| revocation_options=RevocationOptions(enabled=True), | ||
| ) | ||
|
|
||
| assert metadata == snapshot( |
There was a problem hiding this comment.
I've added inline-snapshot as a test dependency. It's super useful.
|
The tests seem to be relying on global state, making |
|
4 participants
Summary
Fixes OAuth endpoint URL construction to always join paths correctly, preventing missing or double slashes regardless of the issuer URL format. Removes unnecessary use of
lstrip("/"), which caused endpoints like$MY_ISSUERauthorizeinstead of the expected$MY_ISSUER/authorize.Motivation and Context
The output of
/.well-known/oauth-authorization-serverwas incorrect when the issuer URL included a path or when using URLs likehttp://localhost:8000.Expected:
Actual:
This was due to the use of
lstrip("/")when joining paths, which strips the leading slash and results in malformed URLs.Note:
There is already PR #770 attempting to address this, but it does not handle all cases (e.g., URLs like
http://localhost:8000or issuer URLs with existing paths).How Has This Been Tested?
Tested with various issuer URLs, including:
Verified the output of:
Example output:
{ "issuer": "https://example.com", "authorization_endpoint": "https://example.com/authorize", "token_endpoint": "https://example.com/token", "registration_endpoint": "https://example.com/register", "revocation_endpoint": "https://example.com/revoke" }All endpoints are now correctly formed, with no missing or double slashes.
Breaking Changes
No breaking changes
Types of changes
Checklist
Additional context
Related PRs
lstripin each OAuth endpoint #770 (works partially)@hongkunyoo @pcarleton @Kludex